Skip to content

feat(agent): select Conjur JWT or legacy username/password authenticator - #822

Open
roeezis wants to merge 5 commits into
jetstack:masterfrom
roeezis:split/05-auth-wiring
Open

feat(agent): select Conjur JWT or legacy username/password authenticator#822
roeezis wants to merge 5 commits into
jetstack:masterfrom
roeezis:split/05-auth-wiring

Conversation

@roeezis

@roeezis roeezis commented Aug 23, 2026

Copy link
Copy Markdown

Summary

Part 5 of the SMS/Conjur JWT authentication series (split out of #817). Stacked on the prior 4 PRs in this series — diff will shrink as they merge.

Adds NewRequestAuthenticator, choosing between the new Conjur JWT exchange and the legacy CyberArk Identity username/password login based on which config is present — Conjur JWT takes priority when both are set. Conjur JWT requires service_id and resolves its base URL from the secrets_manager service discovered in #820, not identity_administration; the two are different hosts.

Switches keyfetch's client over to the new authenticator selection instead of constructing a username/password identity client directly, so JWKS fetching supports both authentication methods too.

Test plan

  • go build ./...
  • go test ./internal/cyberark/... ./internal/envelope/...

@mladen-rusev-cyberark mladen-rusev-cyberark left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The selection logic is clean and auth_select_test.go pins the four cases well. But the keyfetch refactor introduces a regression that breaks the GA username/password path — details inline on internal/envelope/keyfetch/client.go.

Two smaller things:

  • ErrMissingEnvironmentVariables now covers exactly one variable, so the name misleads. ErrMissingSubdomain?
  • auth_select_test.go's header comment says pointing a mock at one endpoint "proves which endpoint the code actually called". True for the username/password case, but the Conjur exchange is lazy, so NewDatauploadClient succeeds without ever dialling SecretsManager. It proves selection, which is the valuable part; to prove the endpoint you'd need to call the returned authenticator.

Comment thread internal/envelope/keyfetch/client.go
Comment thread internal/envelope/keyfetch/client_test.go Outdated
Comment thread internal/cyberark/client.go Outdated
Comment thread internal/cyberark/client.go Outdated
@roeezis
roeezis force-pushed the split/05-auth-wiring branch from 7b9a6fb to e723e62 Compare August 24, 2026 11:42
rzisholz added 2 commits August 24, 2026 14:43
Introduces a small, isolated interface for reading a JWT from a file
path — the first piece of the upcoming Conjur JWT authentication path,
split out on its own since nothing else in this PR depends on it yet.
identity.go mixed the shared client/token-cache plumbing with the
CyberArk Identity username/password (UP) login flow. Move the UP-specific
code into username_password.go so the shared plumbing stays easy to find
once a second login mechanism (Conjur JWT) is added alongside it.

No behavior change — pure extraction, plus exporting the mock's success
credentials for other packages' tests.
@roeezis

roeezis commented Aug 24, 2026

Copy link
Copy Markdown
Author

Blocker fixed. identity.Client.AuthenticateRequest is now self-refreshing, matching conjur.Client — it re-logs-in internally once the cached token passes tokenTTL (now a Client field, not a const), using a durable copy of the credentials captured at the last LoginUsernamePassword call. The caller's own password slice is still zeroed exactly as before; only the client's own internal copy persists, and that's not a materially different exposure than today's status quo (the config already holds it in memory for the process's lifetime). A refresh that fails falls back to the cached token rather than failing the request outright.

Also: moved the identityAPI == "" check into the hasUP branch — Conjur JWT never uses it.

Fixed in the same commit as the second finding below.

@roeezis

roeezis commented Aug 24, 2026

Copy link
Copy Markdown
Author

Fixed on both counts:

  • Deleted the false comment.
  • Added testKeyfetchClientWithIdentityAuth and TestClient_FetchKey_UsernamePasswordAuth, covering the legacy path end-to-end through FetchKey — including a case that forces the JWKS cache stale to prove a second fetch re-authenticates and still succeeds (this is exactly the seam the blocker above broke). Made keyfetch's own JWKS-cache TTL a Client field too, matching the pattern already used for conjur/identity, since the test needs to force it stale without a real 15-minute wait.

@roeezis

roeezis commented Aug 24, 2026

Copy link
Copy Markdown
Author

Fixed — hoisted JWTSourceFile/DefaultAccount consts and a shared ValidateJWTSource helper into internal/cyberark, called from both this file and pkg/agent/config.go so the two can't drift on wording again. Dropped the "POC" wording from the operator-facing message: now jwt_source %q is not supported; supported values are "" and "file".

@mladen-rusev-cyberark mladen-rusev-cyberark left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Round 2 — request changes. Every round-1 finding is addressed, and the self-refresh implementation itself is correct: I checked the obvious hazard (doAdvanceAuthentication takes *[]byte) and confirmed it only reads the password, so the client's internal copy survives repeated refreshes. The new legacy-path tests are the right shape.

But the fix introduced a new blocker that's worse than the one it replaces — details inline on internal/cyberark/client.go. Legacy username/password auth now breaks on the first upload rather than an hour in.

Smaller point: the self-refresh is largely moot on the dataupload path, since NewDatauploadClient builds a brand-new identity.Client on every upload — the token cache never survives between cycles and the refresh never fires there. It only does real work for keyfetch's long-lived client. Worth knowing, since it means the retained-credential trade-off buys less than it appears, and it's another argument for hoisting authenticator construction out of the per-upload path.

Comment thread internal/cyberark/client.go
Comment thread internal/cyberark/client.go
Comment thread internal/cyberark/client.go Outdated
rzisholz added 3 commits August 24, 2026 19:34
The Service Discovery API returns several independently-hosted services;
the authn-jwt exchange this PR series adds is served by secrets_manager,
a different host from identity_administration. Add a SecretsManager
field to Services and parse it, so callers have it available — nothing
reads it yet, that lands in a later PR alongside the client that needs
it.

secrets_manager and discoveryContext are deliberately not required here
unlike identity, since not every caller needs them and requiring
secrets_manager would break every existing username/password install on
a tenant not yet onboarded to Conjur — each caller validates what it
needs at its own point of use instead. identity, by contrast, is
required unconditionally: it's present and active for every healthy
tenant, so callers may rely on it without re-checking.

Factor the repeated "find the first active main endpoint" loop into a
mainActiveAPI helper now that there are three near-identical copies.
Exchanges a projected ServiceAccount JWT (via jwtsource) for a Conjur
access token through the authn-jwt endpoint, and authenticates requests
with it as identity.RequestAuthenticator. Nothing wires this in yet —
that's the next PR, once both this and the legacy identity client exist
side by side.

The identity returned for audit tagging is the token's own sub claim
when it can be extracted, falling back to the configured service ID
otherwise. The cache expiry is driven by the token's own exp claim when
present, falling back to a guessed TTL only when it isn't — a fixed TTL
stamped after the exchange returns would otherwise serve a token past
its real expiry under latency or clock skew. Exposes Invalidate() so a
caller that gets a 401 from the resource server can force a fresh
exchange instead of waiting out the cache.
Adds NewRequestAuthenticator, choosing between the new Conjur JWT
exchange and the legacy CyberArk Identity username/password login based
on which config is present — Conjur JWT takes priority when both are
set. Conjur JWT requires service_id and resolves its base URL from the
secrets_manager service discovered in the prior PR, not
identity_administration; the two are different hosts. The identity API
is only required on the username/password path now; Conjur JWT never
uses it. Since service discovery already errors on a missing
identity_administration API before selectAuthenticator ever runs, the
username/password branch no longer re-checks it — that check could
never actually fire.

Switches keyfetch's client over to the new authenticator selection
instead of constructing a username/password identity client directly.
Doing so dropped keyfetch's own per-fetch LoginUsernamePassword call, which
was the only thing keeping the cached identity token from aging out —
identity.Client.AuthenticateRequest never refreshed on its own. Give it
the same self-refreshing behavior conjur.Client already has: it now
re-logs-in internally once its cached token passes tokenTTL (a field,
not a const, so tests can shrink it), using a durable copy of the
credentials it captured at the last LoginUsernamePassword call. A
refresh that fails falls back to whatever's cached rather than failing
the request outright, since the next call will retry.

LoginUsernamePassword no longer zeroes the caller's password slice: it
already keeps its own durable copy (needed for the self-refresh above),
so wiping the caller's copy bought nothing and actively broke callers
that reuse one ClientConfig across multiple logins — cfg.Secret is a
[]byte shared across every upload cycle via NewCyberArk's configLoader
closure, so the second call received an already-zeroed password and
failed to authenticate. Reproduced live before fixing: a second
PostDataReadingsWithOptions call on the username/password path failed
with "Authentication ... has failed" every time.

Hoists the jwt_source validation and the "file"/"conjur" literals this
and the agent config layer both encode separately into shared
JWTSourceFile/DefaultAccount consts and a ValidateJWTSource helper, and
drops the "POC" wording from the operator-facing error.
@roeezis
roeezis force-pushed the split/05-auth-wiring branch from 0c06690 to afbf35e Compare August 24, 2026 16:38
@roeezis

roeezis commented Aug 24, 2026

Copy link
Copy Markdown
Author

Blocker confirmed and fixed — reproduced your exact repro before fixing (same failure). Took the structural fix, not the one-liner: LoginUsernamePassword no longer zeroes the caller's slice at all — it already keeps its own durable copy (needed for the self-refresh from the earlier round), so wiping the caller's copy bought nothing and is exactly what caused this. Added a two-upload regression test on #823 (where the integration test for this path lives) — a single-upload test structurally can't catch this class of bug.

@roeezis

roeezis commented Aug 24, 2026

Copy link
Copy Markdown
Author

Agreed on the contract critique — fixed by removing the caller-side wipe entirely rather than just patching the call site (see the sibling comment on the blocker above). Two owners of one array with an implicit "callee wipes it" rule is exactly what broke; now there's one owner (the Client's internal copy) and the caller's slice is left alone.

@roeezis

roeezis commented Aug 24, 2026

Copy link
Copy Markdown
Author

Fixed — dropped the check, using serviceMap.Identity.API directly. Agreed on both optional follow-ons and added them: a note above discovery.go:198 stating identityAPI is required unconditionally because it's present for every healthy tenant, and reworded envtest.go's comment to cite that contract instead of reading like a workaround.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants